Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Access] Unify subscription id with client message id #6847

Conversation

illia-malachyn
Copy link
Contributor

@illia-malachyn illia-malachyn commented Jan 3, 2025

From commit message:

From now on, we use only 1 id in request/response messages. This id is called subscription_id.

A client may provide subscription_id in subscribe request. If client does not provide it, we generate it ourselves.

Clients that use browsers or other async environments may use subscription_id to correlate response messages with the request ones.

subscription_id is used in all messages related to subscription.

I also removed success field from a response. We include subscription_id field in a resposne in case of OK response.
In case of error response, we include error field.

Closes #6845

@illia-malachyn illia-malachyn self-assigned this Jan 3, 2025
From now on, we use only 1 id in request/response
messages. This id is called `subscription_id`.

A client may provide `subscription_id` in `subscribe` request.
If client does not provide it, we generate it ourselves.

Clients that use browsers or other async environemnts
may use `subscription_id` to correlate response messages
with the request ones.

`subscription_id` is used in all messages related to subscription.

I also remove `success` field from response. We include `subscription_id`
field in a resposne in case of OK response.
In case of error response, we include `error` field.
@illia-malachyn illia-malachyn force-pushed the illia-malachyn/6845-unify-subscription-and-message-id branch from 8d1e427 to e5a91d7 Compare January 3, 2025 13:08
@illia-malachyn illia-malachyn changed the title Illia malachyn/6845 unify subscription and message [Access] Unify subscription id with client message id Jan 3, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 73.87387% with 29 lines in your changes missing coverage. Please review.

Project coverage is 41.07%. Comparing base (a3c2cea) to head (fe70a58).

Files with missing lines Patch % Lines
engine/access/rest/websockets/controller.go 68.51% 14 Missing and 3 partials ⚠️
...ckets/data_providers/mock/data_provider_factory.go 25.00% 3 Missing and 3 partials ⚠️
...st/websockets/data_providers/mock/data_provider.go 0.00% 4 Missing ⚠️
...ss/rest/websockets/data_providers/base_provider.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6847      +/-   ##
==========================================
- Coverage   41.08%   41.07%   -0.01%     
==========================================
  Files        2121     2121              
  Lines      185905   185901       -4     
==========================================
- Hits        76373    76366       -7     
- Misses     103118   103121       +3     
  Partials     6414     6414              
Flag Coverage Δ
unittests 41.07% <73.87%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

engine/access/rest/websockets/models/list_subscriptions.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/models/base_message.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/error_codes.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/data_providers/utittest.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/controller.go Outdated Show resolved Hide resolved
return uuid.New(), nil
}

newID, err := uuid.Parse(id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jribbink what do you think about requiring clients pass UUIDs (32 character hex strings) for the client ids? Is that reasonable or too restrictive?

Copy link
Contributor Author

@illia-malachyn illia-malachyn Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, if we require clients to create subscripion_id on their side, we can go with a simple integer value for subscription_id as a client will have control over random number generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we decided not to require clients to provide valid uuid but rather a random string containing N characters

@illia-malachyn illia-malachyn requested a review from a team as a code owner January 14, 2025 13:30
Client is not required to provide valid UUID for
subscription ID from now on. It can be any string
[1-20] characters long.

If no subscription ID provided, server creates
UUID string for it.
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a few small comments, but otherwise looks good

engine/access/rest/websockets/controller.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/controller.go Show resolved Hide resolved
engine/access/rest/websockets/controller.go Outdated Show resolved Hide resolved
@peterargue
Copy link
Contributor

@Guitarheroua can you take a look at this one

@Guitarheroua
Copy link
Contributor

@Guitarheroua can you take a look at this one

Will double check it

Copy link
Contributor

@Guitarheroua Guitarheroua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Guitarheroua Guitarheroua added this pull request to the merge queue Jan 21, 2025
Merged via the queue into onflow:master with commit a550d37 Jan 21, 2025
56 checks passed
@Guitarheroua Guitarheroua deleted the illia-malachyn/6845-unify-subscription-and-message-id branch January 21, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Access] Unify subscription and client IDs
5 participants